[AIE NFC] Postpipeliner cleanups and refactorings #253
Conversation
martien-de-jong
requested review from
SagarMaheshwari99,
abhinay-anubola,
abnikant,
andcarminati,
gbossu,
khallouh,
konstantinschwarz and
stephenneuendorffer
as code owners
martien-de-jong
force-pushed
the
martien.smallfixes
branch
from
2f11c48 to
3e5d4c7
Compare
There was a problem hiding this comment.
Rationale: It is difficult to give a useful absolute value. for small ResMII we will be trying way beyond usefulness, for larger ResMII it may not be enough. A relative amount works better.
There was a problem hiding this comment.
This also simplifies using a solver only on the first few tries
There was a problem hiding this comment.
schuling -> scheduling
There was a problem hiding this comment.
It would be nice to have some comments on these fields, for future reference.
There was a problem hiding this comment.
Check: those fields are not used now. Are they related to a new feature?
There was a problem hiding this comment.
ScheduleInfo is passed to the strategies. These values are computed as a service to implementors of new strategies.
There was a problem hiding this comment.
Can we have a description of what does feasibleRA means here? I can see that we have some conf2d_bf16 so it is interesting to also see how this one differs from the others.
|
Hi @martien-de-jong, this set of changes looks good! I just left some comments for clarification! |
This is to give full access to the Info array and it's associated parameters
Dump intervals in ascii art
martien-de-jong
force-pushed
the
martien.smallfixes
branch
from
3e5d4c7 to
95dbddf
Compare
| bool TopDown = true; | ||
| bool Alternate = false; | ||
| int Runs = 0; | ||
| ArrayRef<PriorityComponent> Components; |
There was a problem hiding this comment.
Why not own the list of components? Does that help a lot with memory usage?
| struct Configuration { | ||
| int ExtraStages = 0; | ||
| bool TopDown = true; | ||
| bool Alternate = false; |
There was a problem hiding this comment.
Would be nice to document those fields, especially Alternate
| // When we need more slots than we have data predecessors, we have local | ||
| // resource contention that we can safely account for in Earliest. | ||
| if (Count > 0 && Slots.max() > Count) { | ||
| Me.Earliest = std::max(Me.Earliest, PredEarliest + Slots.max() - 1); |
There was a problem hiding this comment.
Checking: Do we have something similar to bias Latest based on successors? I think yes, but my memory is blurry.
| Info.compute(); | ||
| return true; | ||
|
|
||
| // If no node can be scheduled in cycle 0, we must have a circuit that |
There was a problem hiding this comment.
Nit: If no node can be scheduled in cycle 0 after accounting for LCDs, ...
| // represent a valid 'regular' loop schedule. | ||
| if (NStages == 1) { | ||
| LLVM_DEBUG(dbgs() << "PostPipeliner: Degenerate pipeline, NStages=1\n"); | ||
| return false; |
There was a problem hiding this comment.
FYI @andcarminati , I think that will simplify the epilogue scheduling PR.
gbossu
left a comment
gbossu
left a comment
There was a problem hiding this comment.
This looks good, and I think I want it. Let me know when you went through the comments and I will approve :)
Bump with conflict resolution (6)
|
The important bits have been merged in other postpipeliner PRs |
Small logging improvements
Facility to rerun a strategy an arbitrary number of times
Push earliest with 'local' resource conflicts